Do not allow an empty instance of DiffractionObject - require `xarrays` `yarrays` `xtype` #228

bobleesj · 2024-12-14T06:18:16Z

Closes #227

…th, and xtype

codecov · 2024-12-14T06:33:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7b6ee2e) to head (ba4b985).
Report is 36 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #228   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          372       380    +8     
=========================================
+ Hits           372       380    +8

Files with missing lines	Coverage Δ
tests/conftest.py	`100.00% <100.00%> (ø)`
tests/test_diffraction_objects.py	`100.00% <100.00%> (ø)`

src/diffpy/utils/diffraction_objects.py

bobleesj · 2024-12-14T06:37:03Z

src/diffpy/utils/diffraction_objects.py

+
+    def input_data(self, xarray, yarray, xtype, wavelength, scat_quantity="", name="", metadata={}):
+        """
+        Insert a new scattering quantity into the scattering object.


Improved docstrings, previous version:

f""" insert a new scattering quantity into the scattering object Parameters ---------- xarray array-like of floats the independent variable array yarray array-like of floats the dependent variable array xtype string the type of quantity for the independent variable from {*XQUANTITIES, } metadata, scat_quantity, name and wavelength are optional. They have the same meaning as in the constructor. Values will only be overwritten if non-empty values are passed. Returns ------- Nothing. Updates the object in place. """

If we make it private we may want to move the docstring to the constructor? Also, then change "insert a new scattering object blah blah, to instantiate a new scattering object or sthg.

tests/conftest.py

sbillinge

Please see inline. We may want to check it is ok for us to have things like empty string details instead of None that converts to empty string

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T08:09:29Z

src/diffpy/utils/diffraction_objects.py

+
+    def input_data(self, xarray, yarray, xtype, wavelength, scat_quantity="", name="", metadata={}):
+        """
+        Insert a new scattering quantity into the scattering object.


If we make it private we may want to move the docstring to the constructor? Also, then change "insert a new scattering object blah blah, to instantiate a new scattering object or sthg.

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T08:15:43Z

src/diffpy/utils/diffraction_objects.py

-        self.input_data(xarray, yarray, xtype)
+        self._input_xtype = xtype
+        self._set_xarrays(xarray, xtype)
+        self._all_arrays[:, 0] = yarray


It would be cleaner of this was moved into _set_arrays unless there was a reason not to do that because it is reused somewhere or something

tests/conftest.py

sbillinge · 2024-12-14T08:19:43Z

tests/test_diffraction_objects.py

@@ -41,17 +36,17 @@
        {
            "name": "something",
            "scat_quantity": "",
-            "wavelength": None,
-            "xtype": "",
+            "wavelength": 0.71,


Return wavelengths back to None or absent to test that behavior.

sbillinge · 2024-12-14T08:28:05Z

tests/test_diffraction_objects.py

@@ -241,40 +236,6 @@ def test_dump(tmp_path, mocker):


 tc_params = [
-    (
-        {},


Where are these things now getting tested?

pls see test_init_invalid_args below

but I think these are valid aren't they?

@sbillinge I think these aren't valid since we DiffractionObject requires x, y, and xtype (current PR)

bobleesj · 2024-12-14T17:06:37Z

Please see inline. We may want to check it is ok for us to have things like empty string details instead of None that converts to empty string

@sbillinge Before making PRs, when would be beneficial to have none for xtype or scat_quantity?

My understanding is that we usually want to pass typed variables so that typecasting can be minimal.

Extra comment:

I can see that having wavelength=none makes sense since we don't want a float value associated when it's not provided.

sbillinge · 2024-12-14T17:23:31Z

Please see inline. We may want to check it is ok for us to have things like empty string details instead of None that converts to empty string

@sbillinge Before making PRs, when would be beneficial to have none for xtype or scat_quantity?

I can't think of a case where we want none for xtype. I think we require this, right? For scat_quantity, the tradeoff is between making these easier to use (and therefore encouraging adoption) vs. making them stricter so we collect better metadata. I think leaning in the direction of adoption may be a good starting point for that (though if they are adopted, we may regret that later). That is the UC for a None default for scat_quantity: lazy users....

My understanding is that we usually want to pass typed variables so that typecasting can be minimal.

The issue is with setting mutable objects as defaults. In that case you should pass None and then you have logic like, if thing is None: thing = [] or whatever. I was being a bit too conservative about the empty strings. i was thinking they are mutable since they can act like lists, but I checked and they are not subject to this issue. So we could directly set empty strings (and of course floats and ints) as defaults.

Extra comment:

I can see that having wavelength=none makes sense since we don't want a float value associated when it's not provided.

yes, in this case None is different from 0. We want None here.

bobleesj · 2024-12-14T18:44:28Z

src/diffpy/utils/diffraction_objects.py

+        self.metadata = metadata
+        self.name = name
+        self._input_xtype = xtype
+        self._set_arrays(xarray, xtype, yarray)


as discussed - private func

src/diffpy/utils/diffraction_objects.py

tests/test_diffraction_objects.py

bobleesj

@sbillinge This is ready for review - pls see in-line comments..

bobleesj · 2024-12-14T19:08:45Z

(Just added news)

bobleesj

@sbillinge re-revisiting after resolving a conftest.py conflict. and ready for review.

sbillinge

I reviewed this yesterday or so but it looks as if I forgot to submit the review.....sorry.

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T19:37:55Z

src/diffpy/utils/diffraction_objects.py

+    >>> x = np.array([0.12, 0.24, 0.31, 0.4])  # independent variable (e.g., q)
+    >>> y = np.array([10, 20, 40, 60])  # intensity values
+    >>> metadata = {
+    ...     "package_info": {"version": "3.6.0"}


this could only have been come up with by a programmer. Could we instead have an example of what we are after from materials scientists. Model some good behavior. Something like metadata = {'sample': "rock salt from the beach", "composition": "NaCl", "temperature": "300 K,", "experimenters": "Phill, Sally" }

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T19:41:15Z

src/diffpy/utils/diffraction_objects.py

        array = self.on_xtype(xtype)[0]
        if len(array) == 0:
            raise ValueError(f"The '{xtype}' array is empty. Please ensure it is initialized.")
        i = (np.abs(array - value)).argmin()
        return i

-    def _set_xarrays(self, xarray, xtype):
+    def _set_arrays(self, xarray, xtype, yarray):


maybe we should stay consistent...x, y, xtype order?

tests/test_diffraction_objects.py

sbillinge · 2024-12-14T19:44:41Z

tests/test_diffraction_objects.py

@@ -241,40 +236,6 @@ def test_dump(tmp_path, mocker):


 tc_params = [
-    (
-        {},


but I think these are valid aren't they?

src/diffpy/utils/diffraction_objects.py

bobleesj · 2024-12-16T02:31:39Z

@sbillinge ready for review!

sbillinge · 2024-12-16T10:51:24Z

There is a lot of good stuff on here and this is really getting wrestled into shape in my mind so I want to merge this (and will). There were two things left to do that I will mae issues for. Thanks for your work on this. It is really great!

bobleesj added 8 commits December 14, 2024 01:09

Add do_minimal setup to conftest

92a2c4c

Refactor and remove input_data func

56541e5

Re-organize test with required parameters of xarray, yarray, waveleng…

1144d1f

…th, and xtype

Add input public data back to init func

e7b4dc2

Remove print diff function in pytest for debugging

6dc2738

Update input_data docstrings for clarity

50b7d0d

Add quotation marks around q, tth, d in docstrings

291cc89

Use empty string for default scattering quantity

b8a9adc

Final cleanup on input_data docstrings

e3c5208

bobleesj commented Dec 14, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Outdated Show resolved Hide resolved

bobleesj commented Dec 14, 2024

View reviewed changes

tests/conftest.py Show resolved Hide resolved

bobleesj mentioned this pull request Dec 14, 2024

Do not allow an empty instance of DiffractionObject - init requires xarray, yarray, xtype #227

Closed

sbillinge reviewed Dec 14, 2024

View reviewed changes

bobleesj mentioned this pull request Dec 14, 2024

Refactor test_on_xtype() using @pytest.mark.parametrize and conftest.py #229

Closed

Merge branch 'main' into no-empty-object

db89596

bobleesj changed the title ~~Do not allow an empty instance of DiffractionObject~~ Do not allow an empty instance of DiffractionObject - require xarrays yarrays xtype Dec 14, 2024

bobleesj and others added 4 commits December 14, 2024 13:41

make wavelength optional, test invalid init args

de6f12d

Add wavelength user behavior back

7ccebe8

[pre-commit.ci] auto fixes from pre-commit hooks

2752578

Restore wavelenght none in one of the test cases

d1f767b

bobleesj commented Dec 14, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Show resolved Hide resolved

Fix typo in docstrings

afe814b

bobleesj commented Dec 14, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Show resolved Hide resolved

bobleesj commented Dec 14, 2024

View reviewed changes

tests/test_diffraction_objects.py Show resolved Hide resolved

Use refined param names for pytest parametrize

46c7be8

bobleesj commented Dec 14, 2024

View reviewed changes

bobleesj marked this pull request as ready for review December 14, 2024 19:01

Add news for requires 3 input parameters of , ,

e285214

bobleesj mentioned this pull request Dec 14, 2024

Refactor test-on-xtype test function #231

Merged

bobleesj and others added 2 commits December 15, 2024 14:05

Merge branch 'main' into no-empty-object

4f54439

[pre-commit.ci] auto fixes from pre-commit hooks

7a2603e

bobleesj commented Dec 15, 2024

View reviewed changes

bobleesj mentioned this pull request Dec 15, 2024

docs: Double-check and refine docstrings before 3.6.0 release #239

Closed

sbillinge reviewed Dec 16, 2024

View reviewed changes

bobleesj and others added 7 commits December 15, 2024 21:12

Add metadata data by materials scientist

8693153

Add name example to diffraction object example

e68b59e

Order x, y, xtype for _set_arrays

235f624

Remove 0.71 harded coded wavelength

88ea61b

Add wavelength of 0.711 and 0.71 back

6b20c1a

[pre-commit.ci] auto fixes from pre-commit hooks

9f0f11a

Final setup of wavelenth value comparison

aa1af39

bobleesj commented Dec 16, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Show resolved Hide resolved

sbillinge added 3 commits December 16, 2024 05:36

news tweak

d09d7fb

tweak name example

0e362c1

tweak array length error message

c142240

sbillinge approved these changes Dec 16, 2024

View reviewed changes

sbillinge and others added 3 commits December 16, 2024 05:53

fix missing quote

24b1bbc

fix error message check

2c51af1

[pre-commit.ci] auto fixes from pre-commit hooks

ba4b985

sbillinge merged commit da32ae3 into diffpy:main Dec 16, 2024
2 of 3 checks passed

bobleesj deleted the no-empty-object branch December 16, 2024 11:24

bobleesj mentioned this pull request Dec 26, 2024

revisit the invalid test DiffractionObject #241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow an empty instance of DiffractionObject - require `xarrays` `yarrays` `xtype` #228

Do not allow an empty instance of DiffractionObject - require `xarrays` `yarrays` `xtype` #228

bobleesj commented Dec 14, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading

bobleesj Dec 14, 2024

sbillinge Dec 14, 2024

bobleesj Dec 14, 2024

sbillinge left a comment

sbillinge Dec 14, 2024

sbillinge Dec 14, 2024

bobleesj Dec 14, 2024

sbillinge Dec 14, 2024

sbillinge Dec 14, 2024

bobleesj Dec 14, 2024

sbillinge Dec 14, 2024

bobleesj Dec 16, 2024

bobleesj commented Dec 14, 2024 •

edited

Loading

sbillinge commented Dec 14, 2024

bobleesj Dec 14, 2024

bobleesj left a comment •

edited

Loading

bobleesj commented Dec 14, 2024

bobleesj left a comment

sbillinge left a comment

sbillinge Dec 14, 2024

bobleesj Dec 16, 2024

sbillinge Dec 14, 2024

bobleesj Dec 16, 2024

sbillinge Dec 14, 2024

bobleesj commented Dec 16, 2024

sbillinge commented Dec 16, 2024

Do not allow an empty instance of DiffractionObject - require xarrays yarrays xtype #228

Do not allow an empty instance of DiffractionObject - require xarrays yarrays xtype #228

Conversation

bobleesj commented Dec 14, 2024 • edited Loading

codecov bot commented Dec 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Dec 14, 2024 • edited Loading

sbillinge commented Dec 14, 2024

Choose a reason for hiding this comment

bobleesj left a comment • edited Loading

Choose a reason for hiding this comment

bobleesj commented Dec 14, 2024

bobleesj left a comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Dec 16, 2024

sbillinge commented Dec 16, 2024

Do not allow an empty instance of DiffractionObject - require `xarrays` `yarrays` `xtype` #228

Do not allow an empty instance of DiffractionObject - require `xarrays` `yarrays` `xtype` #228

bobleesj commented Dec 14, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading

bobleesj commented Dec 14, 2024 •

edited

Loading

bobleesj left a comment •

edited

Loading